fix(worker): bug-bash batch — email-forwarder drop, unbounded queries, lease/ctx safety - #80
Merged
Merged
Conversation
…, lease/ctx safety BUG 2 (HIGH) — orphan_sweep_reconciler.fetchLiveStackIDs loaded the ENTIRE stacks table into one unbounded SELECT/result set. Replace with keyset pagination (WHERE id::text > $1 ORDER BY id::text ASC LIMIT $2, batches of orphanLiveIDsBatchLimit=1000). The complete live-id set is still returned — PASS 5 needs every id to avoid deleting a live namespace — but it is now assembled incrementally so the server-side cursor + per-fetch allocation stay bounded regardless of table size. Keyset (not OFFSET) rides the PK index and is stable under concurrent inserts; a stack inserted mid-sweep can only ever PRESERVE a namespace, never wrongly delete one. Tests: keyset multi-page iteration (full page 1 forces a second query whose cursor == page 1 tail), second-page query error, and rows.Err() mid-stream both propagate (no partial set). fetchLiveStackIDs now 100% covered. Updated the four existing PASS 5 / fetch-stack-ids query expectations to the new SQL. NOT CHANGED (verified already-fixed or intentional in current master): - BUG 1 (event_email_forwarder missing-renderer): master already writes a forwarder_sent permanent_drop ledger row BEFORE advancing the cursor (lines ~740-773), with full coverage in TestEventForwarder_MissingRenderer_LoudErrorDropAndAdvance. The brief described pre-fix behavior; nothing to do. - propagation_runner lease: D22-P3 (2026-05-21) already bumps next_attempt_at by propagationLeaseDuration inside the pick transaction — the crash-safe lease the brief asks for. - customer_backup_runner finalize ctx: the fresh context.Background() is the P2-W4 (2026-05-18) FIX, not a bug — using parentCtx would re-strand the row at 'running' mid-shutdown after the S3 object is durable. Reverting would reintroduce the documented bug. SKIPPED (behavior-changing / non-trivial, out of scope for a clean batch): - billing_reconciler keyset: the LIMIT 100 is an intentional per-tick Razorpay-rate-limit fan-out cap, not a pure memory bound; adding keyset pagination to drain all teams per tick changes documented fan-out behavior. - orphan_sweep PASS 6 timeout: per-namespace timeout derives from the job ctx and fails-open by design; job-level time budgeting is governed by River JobTimeout, restructuring it is out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mastermanas805
enabled auto-merge (squash)
June 3, 2026 18:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bug-bash batch for the worker. One genuine open bug fixed (BUG 2); the other findings were verified already-fixed or intentional in current
masterand are documented below so they aren't re-reported.Fixed — BUG 2 (HIGH): unbounded
SELECTinorphan_sweep_reconciler.fetchLiveStackIDsfetchLiveStackIDsloaded the entirestackstable into one result set/allocation (SELECT id::text FROM stacks). Replaced with keyset pagination (WHERE id::text > $1 ORDER BY id::text ASC LIMIT $2, batches oforphanLiveIDsBatchLimit = 1000).Tests added (
fetchLiveStackIDsnow 100% covered):TestOrphanSweep_FetchLiveStackIDs_KeysetPagination— full page 1 (== limit) forces a second query whose cursor arg$1== page 1's tail id; both pages merge into the set.TestOrphanSweep_FetchLiveStackIDs_SecondPageError— a DB error on a later page propagates (no partial set).TestOrphanSweep_FetchLiveStackIDs_RowsErr— arows.Err()mid-stream propagates.Not changed — already fixed / intentional in
masterforwarder_sentpermanent_dropledger row before advancing the cursor (event_email_forwarder.go~740-773), covered byTestEventForwarder_MissingRenderer_LoudErrorDropAndAdvance. The brief described pre-fix behavior.pickEligiblebumpsnext_attempt_atbypropagationLeaseDurationinside the pick transaction — the crash-safe lease requested.context.Background()is the P2-W4 fix (2026-05-18), not a bug. UsingparentCtxwould re-strand the row atrunningmid-shutdown after the S3 object is durable — reverting reintroduces the documented bug.Skipped — behavior-changing / non-trivial
LIMIT 100is an intentional per-tick Razorpay-rate-limit fan-out cap, not a pure memory bound. Keyset-draining all teams per tick changes documented fan-out behavior — out of scope for a clean batch.JobTimeout. Restructuring the sweep's time budget is non-trivial.Gate
make gategreen (build + vet +go test ./... -short -count=1, the exact CI deploy.yml test step). All 16 packages pass.🤖 Generated with Claude Code